Winforms detailed list revamp#4319
Conversation
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks - this is impressive stuff, and the complex bits are all well documented.
I've flagged a couple of minor things that stood out from a review. The only other thing that stood out is refresh handling. We should have some mechanism for initiating a refresh. Two easy approaches would be:
- A Qt-style "header bar" that has a refresh button; or
- Adding a "refresh" option to the context menu.
I don't have strong feelings one way or the other on that one. Longer term, I think we're going to need to reconsider how refresh works; but for now, pragmatism definitely wins.
Regarding the mouse message issue - I'm not aware of any issues with UAC eating message, but I can't rule that out. The other possibility (and it's one that we've seen a lot in the past) is processing speed - it's not uncommon for tests to fail because events are being processed slightly faster or slower under certain testing circumstances. Looking at the select/deselect tests in particular, they're failing right after a "redraw"; adding a small delay to that redraw might help, as might an explicit "wait until selection" loop (i.e., putting the testbed into a "while selection not updated" loop with a longer timeout).
If that doesn't help, I'm willing to be pragmatic; if we're able to test most of the widget, with a couple of small gaps (for some measure of small), I can probably live with that - although it would not be my preferred option. I'd need to see specifics to make a call on how "small" that coverage gap can be.
| if not probe.supports_deselect: | ||
| pytest.skip("The probe for this backend doesn't support deselection.") |
There was a problem hiding this comment.
This works; however, when there's a clear method we can call that wraps the "unsupported" behavior, we usually wrap the skip logic into that method. In this case, adding a deselect_all() method to all backends and raising the skip/xfail in that method means one less probe attribute; plus we can differentiate between "is not implemented on this platform yet" and "cannot be implemented on this platform"
There was a problem hiding this comment.
This is fixed now. I went through the Android code again, and the solution blindingly obvious.
| # According to the MicroSoft documentation, an application must call | ||
| # InitCommonControlsEx must before creating a common control. | ||
| self._init_common_controls_ex = cc32_cls.INITCOMMONCONTROLSEX() | ||
| self._init_common_controls_ex.dwSize = sizeof(cc32_cls.INITCOMMONCONTROLSEX) | ||
| self._init_common_controls_ex.dwICC = wc.ICC_LISTVIEW_CLASSES | ||
| InitCommonControlsEx(byref(self._init_common_controls_ex)) |
There was a problem hiding this comment.
Is this something we should be doing as part of the initialization of the app, rather than the widget?
Does it need to be retained as a property of the widget?
There was a problem hiding this comment.
Is this something we should be doing as part of the initialization of the app, rather than the widget?
It doesn't matter where it's called, as long as it's called before the control in question is created. Also, if doesn't matter if this is called multiple times, and the effect is cumulative for different inputs.
Since this particular widget is the only one that needs this to be called, I think it's best to leave it here.
Does it need to be retained as a property of the widget?
No, good catch!
There was a problem hiding this comment.
I think we should clarify the comment a bit, since it's not exactly easy to find that this is specific to ICC_LISTVIEW_CLASSES. "An application must call InitCommonControlsEx for each type of common controls one wants to create before creating an instance of it" may work better as a comment.
I should have added a note for this! I've pretty much sorted out how to do a macOS style refresh. The dynamics are somewhat fiddly, so I think it's best to get the revamp PR nailed down and then to have a separate PR for the refresh. I also like the idea of a refresh option in the context menu (in addition to the scroll refresh). It will be easy to implement and might add to the overall usability since the scroll refresh is a bit of a hidden feature. What do you think?
These are some good suggestions. Its worth getting to the bottom of this! I'll look into this a bit more and maybe send through some debug commits too. |
I'm OK with "pull-to-refresh" being a later feature; but we need something in place right now. Putting it in the context menu is an easy solution, so we might as well go with that. We can decide later whether it's a permanent thing, an optional thing, or something we remove when pull-to-refresh is an option. |
Oliver-Leigh
force-pushed
the
winforms-detailed-list-revamp
branch
from
25d3c19 to
a02059f
Compare
Oliver-Leigh
force-pushed
the
winforms-detailed-list-revamp
branch
2 times, most recently
from
f15300d to
7514d3b
Compare
Oliver-Leigh
force-pushed
the
winforms-detailed-list-revamp
branch
from
720866d to
b94ed30
Compare
/me waits with anticipation 😄
FWIW - I did a local test merging the .NET Core 10 PR into this one (and running on ARM64), and the DetailedList demo worked and the testbed checks all passed. I didn't check coverage, but it's very encouraging that it worked out of the box.
Yeah - dark mode support is a known limitation - see #1957. If this is something moving to .NET Core can address, then that's another good reason to add it. |
I apologize for the interjection -- but general lifecycle question: After we add a WinUI3 backend and bring it to maturity, should we remove the WinForms backend? I'd prefer not, as a) it's mature, b) is working, c) I really loved the retro look, but it's not modern. But I think there's arguments for removing it as well, as WinForms wouldn't really be advantageous after WinUI3 gets added, especially because we already dropped Windows 8- support. If we end up removing it, I think it may be possible for the community to fork the WinForms backend and maintain it for older Windows versions for those who need it. |
Firstly - at least for now, that's firmly in the category of "problems we'd like to have". Worrying about the deprecation process for a backend that doesn't even exist yet is borrowed trouble. However - I don't imagine we'd actually remove anything until such time as it became problematic to maintain. We might change the default backend (so |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
A couple of minor consistency issues and cleanups flagged inline - but otherwise, I can't fault much here. Nice work!
|
|
||
| @property | ||
| def row_count(self): | ||
| LVM_GETITEMCOUNT = 0x1000 + 4 |
There was a problem hiding this comment.
Any reason to redefine this constant, rather than use the imported version from window constants? (Similarly for a couple of other LVM constants below)
There was a problem hiding this comment.
If redefining constants is not deemed desirable, we should file a ticket for the code in #4276 to import from windowconstants.py as well. I'll write that ticket up if it's deemed desirable here.
There was a problem hiding this comment.
I think I was trying to get a bit of separation/visibility, but it's a bit inconsistent. I've used the ones from the lib file now.
|
Thanks for those updates - they all looks great. The only hiccup standing in the way of a merge is a single test failure that has been introduced by merging with main after landing .NET Core 10 support - it looks like a C function prototype might have changed between .NET Framework 4.x and .NET Core 10? I haven't had a chance to dig into this in depth yet; if you get a chance, that would be most appreciated. |
@freakboy3742 It's a strange place for an error. I'll have a look! |
Oliver-Leigh
force-pushed
the
winforms-detailed-list-revamp
branch
from
00578e5 to
903322a
Compare
|
I tracked down what was happening. It turns out that after the test was completed, the ListView UI was trying to access an uncached item (the next item off the screen). I don't know why this this is happening for this particular mixture of PRs. Perhaps .NET 10 has a more aggressive caching mechanism than the old .NET version. Anyway, I made this a non-issue by reducing the number of children on the root from 50 to 10. There was also a hole in the coverage which I fixed with a slight restructuring. |
johnzhou721
left a comment
johnzhou721
left a comment
There was a problem hiding this comment.
I'm slightly concerned about the reduction of the number of items in the tree test. Do you have any idea about the exact cause .NET 10 is doing the out of bounds access, because perhaps it could point to a real bug somewhere?
Actually, it's not the DetailedList PR which is causing the uncached item to be accessed. It is caused by #4331 (or earlier). You can observe this by running the testbed in slow mode on the current Toga version. On my PC I see the warning message printed out, however it is not raised to an error. There is something about the DetailedList PR which is causing this warning to be raised (correctly) to an error. It's difficult to track down exactly what, since neither are raised as errors on my PC. Perhaps the bigger issue here is why this warning has not been raise to an error in the earlier PR. |
I think the issue here may be to track down exactly what is causing that warning/error, and just fix it (by disposing the appropriate caches when the widget is destroyed in Python, perhaps?). I'm not sure if it's warranted to debug this right now, or we should file a ticket to track it down, using the workaround.
Perhaps as you've mentioned earlier, you have a manifest file on your PC somewhere in your chain? Perhaps try to replicate this on a clean Windows box? Anyways: I'm exiting this conversation now, given I'm not familiar with Windows. The timeline for debugging this is up to @freakboy3742 and/or other core team members. |
That's not entirely true - because #4331 (and the current state of main) passes CI. At the very least, there's a complex memory interaction - so it's entirely possible that .NET 10 has exposed a different set of interactions.
Frustratingly, I don't see the error locally... I'll admit I'm not wild about "make the test smaller" as a fix... but it's a single test that is being tweaked, I can't see anything being tested that is obviously size dependent, and there's no evidence that the test fails "in the wild"; so I'm cautiously willing to accept the reduction in size as a pragmatic measure (since the improvement of adding a real DetailedList is significant), and the testbed is known to have some weird interactions with memory that aren't reflective of the "real world". If we're able to reproduce this reliably in any other context, then we can open a ticket on that basis. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for all your work on this - this is a big feature bump that we've wanted for a long time!
You're welcome! |
I agree it's not the best. It's most likely to do with some asynchronous disposal issue. I think I'm seeing the warning after the test is finished (so no exception is thrown), but the testbed is seeing the warning before the test is finished. In any case, we'll see it this issue pops up again. |
3 participants
A revamp of the DetailedList widget for Windows
The revamp of the DetailList widget has two key advantages over the existing implementation:
How it works:
The widget is based on the Win32 List-View UI using the tile view option. The reason for using a Win32 approach here is that the WinForms List-View UI does not support tile view in virtual mode.
The tile view natively places the tiles into rows, so here the tiles are resized so that there is only one column. Unfortunately, there is a bug in tile view where the tile sizes can become "stuck" upon too many resizes. Much of the code here is to overcome cascading issues resulting from this.
The approach taken to overcome the resize bug is to completely custom-draw the tiles.
The tiles and icons automatically resize themselves based on the size for the font used. Since the fonts are resized on DPI changes, the widget will adjust itself to DPI changes.
The actions are implemented using a context menu à la macOS. A Win32 approach is used for the context as well. This is because the style of the WinForms context menu has not been updated for recent version of Windows (see, for example, dotnet/winforms#2476).
A note about the tests
I needed to add a deselect test to get full code coverage. My current setup can't run the new tests and I was unable to write this test for Android. So the tests will most likely need some fixing/adjustments.
It seems that the online Windows testbed is blocking the mouse button Windows messages. I've tried a few work arounds, but I think it might be some UAC settings that are blocking them. Maybe this is easily fixed? Otherwise I can rework the tests and add in some extra praga: no cover statements.
PR Checklist: